client: report last connection error to RPCs via v1 balancer API #2508
Conversation
jeanbza
changed the title
| @@ -315,12 +313,12 @@ func (bw *balancerWrapper) Pick(ctx context.Context, opts balancer.PickOptions) | |||
| Metadata: a.Metadata, | |||
| }] | |||
| if !ok && failfast { | |||
There was a problem hiding this comment.
@menghanl this is a bug, right? We should return an error regardless of failfast, shouldn't we? Otherwise, we'll be trying to use sc (which will be nil) in the lookup below, and then eventually returning it from the function with no error.
There was a problem hiding this comment.
Returning nil will not cause error because we check it. nil will result in a re-pick.
We will only get nil here when things are not in sync (between the wrapper and the v1-balancer). This means the connectivity state has changed, and a new pick was (or will be) updated. So I think re-pick makes sense.
There was a problem hiding this comment.
But we will also get a re-pick if we return the transient failure error immediately. Technically this works as-is, but if an error happens we should be returning an error, not a nil sc with no error.
Returning a nil sc will also result in the info log (which should probably be a warning) "subconn returned from pick is not *acBalancerWrapper", which shouldn't normally be happening (and it puts our implementation details into the user's log messages).
OK, let's go ahead with this PR but I think we should change this separately. I'll send a PR.
| @@ -315,12 +313,12 @@ func (bw *balancerWrapper) Pick(ctx context.Context, opts balancer.PickOptions) | |||
| Metadata: a.Metadata, | |||
| }] | |||
| if !ok && failfast { | |||
There was a problem hiding this comment.
But we will also get a re-pick if we return the transient failure error immediately. Technically this works as-is, but if an error happens we should be returning an error, not a nil sc with no error.
Returning a nil sc will also result in the info log (which should probably be a warning) "subconn returned from pick is not *acBalancerWrapper", which shouldn't normally be happening (and it puts our implementation details into the user's log messages).
OK, let's go ahead with this PR but I think we should change this separately. I'll send a PR.
Now that grpc/grpc-go#2508 is in, gRPC plumbs the last connection error when we attempt to make an RPC on a clientconn that has not been successfully conencted. So, in this CL, we check errors for a permanent connection problem. Specifically if certificates are misconfigured, or ca-certificates are missing, we expect not to be able to establish a connection in a reasonable amount of time and instead bail out and return the error to the user to fix. Fixes googleapis/google-cloud-go#1234
dfawley
changed the title
Now that grpc/grpc-go#2508 is in, gRPC plumbs the last connection error when we attempt to make an RPC on a clientconn that has not been successfully conencted. So, in this CL, we check errors for a permanent connection problem. Specifically if certificates are misconfigured, or ca-certificates are missing, we expect not to be able to establish a connection in a reasonable amount of time and instead bail out and return the error to the user to fix. Fixes googleapis/google-cloud-go#1234
Now that grpc/grpc-go#2508 is in, gRPC plumbs the last connection error when we attempt to make an RPC on a clientconn that has not been successfully conencted. So, in this CL, we check errors for a permanent connection problem. Specifically if certificates are misconfigured, or ca-certificates are missing, we expect not to be able to establish a connection in a reasonable amount of time and instead bail out and return the error to the user to fix. Fixes googleapis/google-cloud-go#1234
Now that grpc/grpc-go#2508 is in, gRPC plumbs the last connection error when we attempt to make an RPC on a clientconn that has not been successfully conencted. So, in this CL, we check errors for a permanent connection problem. Specifically if certificates are misconfigured, or ca-certificates are missing, we expect not to be able to establish a connection in a reasonable amount of time and instead bail out and return the error to the user to fix. Fixes googleapis/google-cloud-go#1234
No description provided.